Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Plotly Dash support #4605

Merged
merged 11 commits into from
Nov 7, 2020
Merged

Add Plotly Dash support #4605

merged 11 commits into from
Nov 7, 2020

Conversation

jonmmease
Copy link
Collaborator

This PR adds support for displaying dynamic HoloViews objects using Plotly Dash.

The new logic is localized to the single holoviews/plotting/plotly/dash.py file which provides a new holoviews_to_dash function.

A new section has been added to all of the dynamic Plotly reference guide notebooks that includes the code for displaying the example in a Dash app. This is a good place to look at the example usage for the holoviews_to_dash function.

This PR is dependent on some of the fixes in #4572, and so it should be rebased on master after #4572 is merged.

@jonmmease jonmmease marked this pull request as draft September 11, 2020 16:16
@@ -0,0 +1,2 @@
[pytest]
addopts = -p no:dash
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was added to disable the dash pytest plugin, which was causing this error (https://travis-ci.org/github/holoviz/holoviews/jobs/731249907):

pluggy.manager.PluginValidationError: Plugin 'dash' could not be loaded: (dash-table 4.4.1 (/home/travis/miniconda/envs/test-environment/lib/python2.7/site-packages), Requirement.parse('dash-table==4.10.1'))!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to add dash-table to the testing envt so that the plugin doesn't have to be disabled?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did try that, but the version on the main anaconda channel is too old (4.4.1). We'd have to pull 4.10.* from conda-forge, and it doesn't look like any packages are pulled from conda-forge right now.

@jonmmease jonmmease marked this pull request as ready for review September 29, 2020 13:33
@jonmmease
Copy link
Collaborator Author

I rebased on master and this PR is ready for review.

],
"metadata": {
"collapsed": false,
"pycharm": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder if this metadata is why GitHub isn't able to preview these notebooks? I recommend just clearing it out...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good point. I'll clear it out if we decide to keep these notebook changes

@jlstevens
Copy link
Contributor

jlstevens commented Sep 29, 2020

Looks great!

Not much to say other than I would consider naming holoviews_to_dash to simply to_dash - anything imported from holoviews is implicitly about holoviews already :-)

Copy link
Member

@jbednar jbednar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I have a few small suggestions, and generally a wish for consolidating the Dash examples to avoid bloating the Reference gallery (see comments), but it will be great to have Dash support!

I think I've seen a demo of Dash supporting other libraries like Matplotlib and Bokeh, though I don't know how solid that was. I wonder if the mpl backend for HoloViews could be supported with Dash fairly easily, because it wouldn't need to be very ambitious (just displaying an image). Bokeh would be more tricky, I assume.

"import dash_html_components as html\n",
"\n",
"# Create App\n",
"external_stylesheets = ['https://codepen.io/chriddyp/pen/bWLwgP.css']\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to avoid depending on code in anyone's personal accounts; does it really need the .css decoration? Or could the file be moved somewhere more permanent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, yeah, I'll pull that out if we keep the examples in these notebooks.

Comment on lines 91 to 92
"app.layout = html.Div(\n",
" [components.graphs[0], components.resets[0], components.store]\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't suppose there's a way to eliminate this step? Is it not appropriate for to_dash to modify app to do that already, at least as a default (with the default overridden in some examples that need special widgets)?

Overall, this is a lot of code to be adding to each of the examples, when most of it seems boilerplate. Can we not instead just make a single page in the user guide that shows one Panel example, one Voila example, and one Dash example (plus the caveat that Dash will only work for the Plotly backend), and then have all the reference notebooks (for all backends) link back to that page? That seems easier for us to maintain plus throws less code at the user to try to follow (assuming any given user will click on many of these pages).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't suppose there's a way to eliminate this step? Is it not appropriate for to_dash to modify app to do that already, at least as a default (with the default overridden in some examples that need special widgets)?

I don't think so, the idea is that user should place them in the layout of a larger Dash app that includes other Dash components.

I'll make some comments about the docs down below

@@ -0,0 +1,2 @@
[pytest]
addopts = -p no:dash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to add dash-table to the testing envt so that the plugin doesn't have to be disabled?

holoviews/plotting/plotly/dash.py Outdated Show resolved Hide resolved
if panel_prop == "selected_data":
if graph_id + ".selectedData" in triggered_prop_ids:
# Only update selectedData values that just changed.
# This way we don't the the may have been cleared in the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

???

@jonmmease
Copy link
Collaborator Author

Not much to say other than I would consider naming holoviews_to_dash to simply to_dash

Sure, works for me

@jonmmease
Copy link
Collaborator Author

Regarding the documentation. I'm currently working on a documentation page that will be added to the main Dash docs with a structure similar to https://dash.plotly.com/layout. I've waffled on this a little bit, but I'm currently leaning towards including Dash versions of these 6 interactive Plotly examples in that documentation page. The nice thing about having them there is that they will be fully interactive in the documentation.

So the simplest path forward may be for me to revert the notebook changes in this PR, then after the Dash documentation is published (which will require these changes to be released in HoloViews), add links from these pages to the corresponding example in the Dash docs.

How does that sound? The shared Panel/Voila/Dash page makes sense, but I guess I'm not volunteering to write it for this PR 🙃

@jlstevens
Copy link
Contributor

jlstevens commented Sep 30, 2020

Linking to plotly docs from the reference notebooks seems reasonable to me. My other suggestion is that instead of putting code into those notebooks and only having links, a section could be added to the Plotting with Plotly user guide but unfortunately it looks there was a problem with doc building :-(

image

A shared Panel/Voila/Dash user guide section would also be nice, but I am only proposing a dash example of the sort you added to the reference notebooks.

@jonmmease
Copy link
Collaborator Author

Linking to plotly docs from the reference notebooks seems reasonable to me.

Ok, I'll go ahead and revert the notebook changes from this PR and then come back to it after the Dash docs are up.

... Plotting with Plotly ...

Looks like there isn't an rst file for Plotting with Plotly

Screenshot_20200930_051430

Do you think this existed at one point? Or maybe was never written?

@jlstevens
Copy link
Contributor

jlstevens commented Sep 30, 2020

Do you think this existed at one point? Or maybe was never written?

Good question! As far as I remember, those .rst files need to be explicitly added and I see no reason why such a file would ever have been removed. Could you add one for plotly in this PR? Following the Bokeh example, it should just be:

Plotting with Plotly
____________________

.. notebook:: holoviews ../../examples/user_guide/Plotting_with_Plotly.ipynb
    :offset: 1

If you decide to update that notebook, we'll then run a doc build to preview the changes.

@jonmmease
Copy link
Collaborator Author

Updates

  • holoviews_to_dash renamed to to_dash in dd373a9
  • Notebook changes reverted in 59a9381. I'll add links to Dash docs once they are live.

Could you add one for plotly in this PR? Following the Bokeh example, it should just be ...

Looks like a target Plotting_with_Plotly.ipynb notebook was never written. This was raised in #3949. I should write this soon, but I don't think it will be in this PR.

@@ -67,7 +68,7 @@
'mock',
'flake8 ==3.6.0',
'coveralls',
'path.py',
'path.py',
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops, my editor 🔪 the trailing whitespace here automatically, I can revert if anyone cares that it's not related to this PR.

@philippjfr
Copy link
Member

That's right, a Plotting_with_Plotly guide was never actually written.

…mponents

This can be used directly in `html.Div()` if no additional layout structure
is needed
slider_label_id = kdim_name + "-label-" + slider_uuid
kdim_uuids.append(slider_uuid)

html_lable = html.Label(id=slider_label_id, children=kdim_label)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: html_lable ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 772b9ed

@jonmmease
Copy link
Collaborator Author

@kcpevey, is this Ubuntu Python 3.6 build failure a known issue? I don't see any error messages related to the tests that I recognize. Thanks!

@jonmmease jonmmease added this to the v1.14.0 milestone Nov 4, 2020
@kcpevey
Copy link
Collaborator

kcpevey commented Nov 6, 2020

@jonmmease that is not a known failure. I thought perhaps it was a one-off thing so I reran the workflows and now there are 3 failures across different versions/architectures. Based on the error, I don't know what's causing that.

@philippjfr
Copy link
Member

Trying to fix up those flakey tests now.

@jonmmease
Copy link
Collaborator Author

Thanks @philippjfr!

@philippjfr philippjfr deleted the dash_support branch April 25, 2022 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants